fix: keep parent chat running in drawer while a subagent works - #233
Conversation
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
ℹ️ All inline comments overlapped with existing reviews; nothing new was posted. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "session.created" -> { | ||
| val session = | ||
| json.decodeFromJsonElement( | ||
| OpenCodeSession.serializer(), | ||
| properties["info"]!!.jsonObject, | ||
| ) | ||
| OpenCodeEvent.SessionCreated(session) | ||
| } |
There was a problem hiding this comment.
[maintainability · low]
session.created と session.updated の分岐は properties["info"] を OpenCodeSession にデコードするロジックが完全に重複しています。private fun parseSession(properties: JsonObject) = json.decodeFromJsonElement(OpenCodeSession.serializer(), properties["info"]!!.jsonObject) のようなヘルパーを抽出し、各分岐を OpenCodeEvent.SessionCreated(parseSession(properties)) / OpenCodeEvent.SessionUpdated(parseSession(properties)) とすると冗長さが解消され、今後デコード処理を変更する際の差分も1箇所に集約できます。
| var parentId = parentIdOf(target, sessionId) | ||
| while (parentId != null) { | ||
| if (isRuntimeIdle(parentId)) break | ||
| val ancestorId = parentId |
There was a problem hiding this comment.
[bug · medium]
parentIds は runtime のイベント/API 由来のデータをキャッシュしており、万一親チェーンに循環（A→B→A のような不整合データ）が混入すると、isRuntimeIdle による break 条件が成立しない限りこの while ループが無限に回り続けます。ループ内では parentIdOf がネットワーク呼び出しを伴う可能性があり、さらに mutableState.update が連続実行されてイベント収集コルーチンがハングします。visited 集合で訪問済みセッションを検知する防御を追加してください。
Suggestion:
| var parentId = parentIdOf(target, sessionId) | |
| while (parentId != null) { | |
| if (isRuntimeIdle(parentId)) break | |
| val ancestorId = parentId | |
| var parentId = parentIdOf(target, sessionId) | |
| val visited = mutableSetOf<String>() | |
| while (parentId != null) { | |
| if (!visited.add(parentId)) break | |
| if (isRuntimeIdle(parentId)) break | |
| val ancestorId = parentId |
| parentId: String?, | ||
| ) { | ||
| if (sessionId.isBlank()) return | ||
| synchronized(parentLock) { parentIds[sessionId] = parentId } |
There was a problem hiding this comment.
[bug · medium]
API 解決に失敗した場合も null が parentIds に永続キャッシュされるため、「親なしと確認済み」と「解決失敗」が区別できません。一時的な API エラーで subagent の親解決が失敗すると、以後そのセッションは親なしとして扱われ、SessionIdle 時に onSessionIdle が誤発火したり activateAncestors が祖先を活性化できなくなったりしますが、キャッシュ済みのため再試行もされません。解決に成功した場合のみキャッシュする（失敗時はキャッシュしない）よう修正してください。
Suggestion:
| synchronized(parentLock) { parentIds[sessionId] = parentId } | |
| if (parentId != null || cachedParent(sessionId)) { | |
| synchronized(parentLock) { parentIds[sessionId] = parentId } | |
| } |
| if (sessionId in parentIds) return parentIds[sessionId] | ||
| } | ||
| // Misses the creation event when the stream reconnected mid-run; ask the runtime instead. | ||
| val parentId = runCatching { target.session(sessionId).parentId }.getOrNull() |
There was a problem hiding this comment.
[bug · medium]
target.session(sessionId) は suspend なネットワーク/プロセス呼び出しですが、runCatching は CancellationException も捕捉してしまうため、収集コルーチンがキャンセルされた際にキャンセレーションが握りつぶされ、構造的並行性が壊れます。さらに失敗結果の null が下の行でキャッシュされるため、キャンセル時に誤った親情報が永続化されます。CancellationException は必ず再送出してください。
Suggestion:
| val parentId = runCatching { target.session(sessionId).parentId }.getOrNull() | |
| val parentId = try { | |
| target.session(sessionId).parentId | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| null | |
| } |
| private val parentIds = mutableMapOf<String, String?>() | ||
| private val parentLock = Any() |
There was a problem hiding this comment.
[performance · low]
parentIds と runtimeIdleSessionIds は一度追加されたエントリが削除される経路がなく、セッションが増える・対象ランタイムが切り替わるたびに単調に増加し続けます。markSessionRunning が runtimeIdleSessionIds を除去するのは一部の経路のみで、parentIds に至っては削除処理が一切ありません。長時間の利用でメモリが蓄積するため、セッション削除時やランタイム切替時に不要エントリを掃除する仕組みを検討してください。
| when (event) { | ||
| OpenCodeEvent.ServerConnected -> appendLog(messages.eventConnectedTitle, messages.eventConnectedDetail) | ||
| is OpenCodeEvent.SessionCreated -> rememberParent(event.session.id, event.session.parentId) | ||
| is OpenCodeEvent.SessionUpdated -> rememberParent(event.session.id, event.session.parentId) |
There was a problem hiding this comment.
[bug · low]
SessionUpdated は既存のキャッシュを無条件に上書きします。OpenCodeSession.parentId はデフォルト値 null のため、session.updated の info に parentID が含まれない部分更新だった場合、SessionCreated で得られていた正しい親情報が null で上書きされて失われます。親関係は原則変化しないため、SessionUpdated では非 null のときだけ記録するのが安全です（SessionCreated での null 記録は「親無し確定」なのでそのまま）。
Suggestion:
| is OpenCodeEvent.SessionUpdated -> rememberParent(event.session.id, event.session.parentId) | |
| is OpenCodeEvent.SessionUpdated -> event.session.parentId?.let { rememberParent(event.session.id, it) } |
| private suspend fun activateSession( | ||
| target: RuntimeTarget, | ||
| sessionId: String, | ||
| force: Boolean = false, | ||
| ) { | ||
| if (sessionId.isBlank()) return | ||
| mutableState.update { current -> |
There was a problem hiding this comment.
[bug · medium]
activateSession はセッションを activeSessionIds に追加しますが、runtimeIdleSessionIds からは除去しません。markSessionRunning や SessionStatusChanged(非idle) は markRuntimeRunning で除去しているのに対し、ストリームイベント経由の再活性化だけが除去されず非対称です。一度 idle 扱い（SessionIdle / SessionStatusChanged(idle) / SessionError）されたセッションがメッセージ等のイベントで再開した後、そのセッションを親に持つサブエージェントのイベントが来ると activateAncestors が isRuntimeIdle で即 break し、祖先チェーンが再活性化されません。冒頭で markSessionRunning 同様に idle フラグを除去してください。
Suggestion:
| private suspend fun activateSession( | |
| target: RuntimeTarget, | |
| sessionId: String, | |
| force: Boolean = false, | |
| ) { | |
| if (sessionId.isBlank()) return | |
| mutableState.update { current -> | |
| private suspend fun activateSession( | |
| target: RuntimeTarget, | |
| sessionId: String, | |
| force: Boolean = false, | |
| ) { | |
| if (sessionId.isBlank()) return | |
| synchronized(parentLock) { runtimeIdleSessionIds.remove(sessionId) } | |
| mutableState.update { current -> |
| "session.created" -> { | ||
| val session = | ||
| json.decodeFromJsonElement( | ||
| OpenCodeSession.serializer(), | ||
| properties["info"]!!.jsonObject, | ||
| ) | ||
| OpenCodeEvent.SessionCreated(session) | ||
| } | ||
| "session.updated" -> { | ||
| val session = | ||
| json.decodeFromJsonElement( | ||
| OpenCodeSession.serializer(), | ||
| properties["info"]!!.jsonObject, | ||
| ) | ||
| OpenCodeEvent.SessionUpdated(session) | ||
| } |
There was a problem hiding this comment.
[maintainability · low]
session.created と session.updated の 2 分岐で全く同じデコード処理が重複しています。private ヘルパー関数（例: private fun parseSessionInfo(properties: JsonObject) = json.decodeFromJsonElement(OpenCodeSession.serializer(), properties["info"]!!.jsonObject)）に抽出し、両分岐から呼び出すことで冗長性を排除できます。
Suggestion:
| "session.created" -> { | |
| val session = | |
| json.decodeFromJsonElement( | |
| OpenCodeSession.serializer(), | |
| properties["info"]!!.jsonObject, | |
| ) | |
| OpenCodeEvent.SessionCreated(session) | |
| } | |
| "session.updated" -> { | |
| val session = | |
| json.decodeFromJsonElement( | |
| OpenCodeSession.serializer(), | |
| properties["info"]!!.jsonObject, | |
| ) | |
| OpenCodeEvent.SessionUpdated(session) | |
| } | |
| "session.created" -> OpenCodeEvent.SessionCreated(parseSessionInfo(properties)) | |
| "session.updated" -> OpenCodeEvent.SessionUpdated(parseSessionInfo(properties)) |
| if (sessionId.isBlank()) return | ||
| synchronized(parentLock) { parentIds[sessionId] = parentId } |
There was a problem hiding this comment.
[bug · low]
rememberParent は parentId の空文字列を正規化せず、parentIdOf も session.parentId をそのまま返します。API がトップレベルセッションに対して parentID: "" を返すと、activateAncestors は空文字列を有効なセッションIDとして扱い、activeSessionIds に不正なエントリを追加します。セッションIDには isBlank() チェックを一貫して行っているため、親IDも同様に null へ正規化すべきです（parentIdOf 内の書き込み箇所も同様）。
Suggestion:
| if (sessionId.isBlank()) return | |
| synchronized(parentLock) { parentIds[sessionId] = parentId } | |
| if (sessionId.isBlank()) return | |
| synchronized(parentLock) { parentIds[sessionId] = parentId?.takeIf { it.isNotBlank() } } |
| if (sessionId in parentIds) return parentIds[sessionId] | ||
| } | ||
| // Misses the creation event when the stream reconnected mid-run; ask the runtime instead. | ||
| val session = runCatching { target.session(sessionId) }.getOrNull() ?: return null |
There was a problem hiding this comment.
[bug · medium]
runCatching は CancellationException を含む全ての例外を捕捉します。parentIdOf は suspend 関数であり、コルーチンキャンセル中に target.session() が投げた CancellationException が握りつぶされると、構造的並行性が壊れ、キャンセルが収集コルーチンへ伝播しなくなります（ストリーム再接続や runtime 切替時の終了処理が遅延する恐れ）。CancellationException は再送出するか、try/catch で明示的に扱うべきです。
Suggestion:
| val session = runCatching { target.session(sessionId) }.getOrNull() ?: return null | |
| val session = | |
| try { | |
| target.session(sessionId) | |
| } catch (e: kotlinx.coroutines.CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| return null | |
| } |
| private fun markRuntimeIdle(sessionId: String) { | ||
| synchronized(parentLock) { runtimeIdleSessionIds.add(sessionId) } | ||
| } |
There was a problem hiding this comment.
[bug · low]
markRuntimeIdle / markRuntimeRunning / parentIdOf には sessionId の空文字ガードがない。markSessionRunning には isBlank() ガードがあるため一貫性を欠き、空文字が runtimeIdleSessionIds に残ったり target.session("") の無駄な呼び出しが発生したりする。空文字はガードして早期 return することを推奨します。
| is OpenCodeEvent.SessionCreated -> Unit | ||
| is OpenCodeEvent.SessionUpdated -> Unit |
There was a problem hiding this comment.
[maintainability · low]
SessionUpdated は更新後の OpenCodeSession（title、tokens など）を保持していますが、ここでは破棄されており、sessionTitle や context usage がサーバー側と乖離する可能性があります。特に createSession(title = null) で作成した新規セッションは、最初のメッセージ後に OpenCode がタイトルを自動生成するため、sendMessage 直後の currentBackend.session(...) による更新（L862）だけでは取得できないケースがあり、SessionUpdated で受信したタイトルを反映しないと一覧/ヘッダーが古いままになります。event.session.id == activeSession の場合に sessionTitle や refreshContextUsage の更新へ活用することを検討してください（現状は意図的なno-opであれば問題ありません）。
| "session.updated" -> { | ||
| val session = | ||
| json.decodeFromJsonElement( | ||
| OpenCodeSession.serializer(), | ||
| properties["info"]!!.jsonObject, | ||
| ) | ||
| OpenCodeEvent.SessionUpdated(session) | ||
| } |
There was a problem hiding this comment.
[maintainability · low]
session.created と同一のデコード処理が完全に重複しています。OpenCodeSession へのデコードとプロパティへのアクセスを1つの private ヘルパーに抽出することで、when 文が簡潔になり、将来のスキーマ変更時に修正漏れ（片方だけ更新）を防げます。
Suggestion:
| "session.updated" -> { | |
| val session = | |
| json.decodeFromJsonElement( | |
| OpenCodeSession.serializer(), | |
| properties["info"]!!.jsonObject, | |
| ) | |
| OpenCodeEvent.SessionUpdated(session) | |
| } | |
| "session.updated" -> OpenCodeEvent.SessionUpdated(parseSession(properties)) | |
| // private fun parseSession(properties: JsonObject) = | |
| // json.decodeFromJsonElement( | |
| // OpenCodeSession.serializer(), | |
| // properties["info"]!!.jsonObject, | |
| // ) |
| parentResolutionOf(target, event.sessionId).onSuccess { parentId -> | ||
| if (parentId == null) { | ||
| onSessionIdle?.invoke(event.sessionId, sessionTitle(target, event.sessionId), target.id) | ||
| } | ||
| } |
There was a problem hiding this comment.
[bug · high]
SessionIdle 処理で parentResolutionOf(...).onSuccess { ... } を使ったため、親 ID 解決に失敗した場合に onSessionIdle が呼ばれなくなりました。旧実装では getOrDefault(false) により解決失敗時にトップレベルセッション扱いで完了通知を発火していました。一時的な API エラーやキャンセルで完了通知が恒久的に失われ、UI が実行中表示のまま残るなどの機能退行を引き起こします。失敗時も旧挙動どおりフォールバックして通知するよう検討してください。
Suggestion:
| parentResolutionOf(target, event.sessionId).onSuccess { parentId -> | |
| if (parentId == null) { | |
| onSessionIdle?.invoke(event.sessionId, sessionTitle(target, event.sessionId), target.id) | |
| } | |
| } | |
| val parentId = parentResolutionOf(target, event.sessionId).getOrNull() | |
| if (parentId == null) { | |
| onSessionIdle?.invoke(event.sessionId, sessionTitle(target, event.sessionId), target.id) | |
| } |
| private suspend fun parentIdOf( | ||
| target: RuntimeTarget, | ||
| sessionId: String, | ||
| ): String? = parentResolutionOf(target, sessionId).getOrNull() |
There was a problem hiding this comment.
[bug · medium]
parentIdOf は解決失敗時も null を返すため、activateAncestors の親チェーン探索がそこで中断し、一時的なエラー時にさらに上位の祖先セッションがアクティブ化されません。また失敗時はキャッシュされないため、解決成功までイベント（特にストリーミング中の多数の MessagePartDelta）ごとに API 呼び出しが再実行されます。失敗と「親なし」を区別する、または再試行回数に上限を設けることを検討してください。
| return runCatching { target.session(sessionId).parentId } | ||
| .onSuccess { parentId -> synchronized(parentLock) { parentIds[sessionId] = parentId } } |
There was a problem hiding this comment.
[bug · high]
runCatching は CancellationException を含む全ての Throwable を捕捉します。target.session(...) は suspend のネットワーク呼び出し（get("session/...")）のため、collectLatest によるターゲット切替などのキャンセル時に投げられた CancellationException がここで吸収され、コルーチンのキャンセルが遅延・握りつぶされます。キャンセル後に旧ターゲットのイベント処理が続き mutableState を誤更新する恐れがあります。CancellationException は必ず再送出してください。
Suggestion:
| return runCatching { target.session(sessionId).parentId } | |
| .onSuccess { parentId -> synchronized(parentLock) { parentIds[sessionId] = parentId } } | |
| return try { | |
| Result.success(target.session(sessionId).parentId) | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| Result.failure(e) | |
| }.onSuccess { parentId -> synchronized(parentLock) { parentIds[sessionId] = parentId } } |
A session blocked on the task tool emits no events while its subagent runs. If the user navigated away, the chat reported the parent finished and the drawer settled it on the grey idle dot for the subagent's entire run. Learn each subagent's parent from session.created/session.updated events (falling back to the runtime API when the creation was missed) and forward the child's activity up the parent chain, so the drawer keeps showing the spinner. Ancestors the runtime already reported idle are left alone, so an experimental background subagent cannot resurrect a finished turn.
90391f6 to
53a3caa
Compare
1 participant
Problem
When a session spawns a subagent (the
tasktool), the drawer's session list drops the parent back to the grey idle dot even though the run is plainly still in flight.Reproduced against a live OpenCode 1.18.15 server: while a subagent works, the parent session emits no events at all — its loop is blocked inside the task tool, and every stream event carries the child's session id.
The drawer derives the running state from
RuntimeActivityRepository.activeSessionIds. Two things combine to lose the parent:So the parent sat on the grey dot for the subagent's entire run.
Fix
session.created/session.updatedevents to learn each subagent's parent (falls back to a one-shotsession()lookup when the creation event was missed, e.g. app restart mid-run).No UI changes needed: the drawer already renders
RUNNINGfor anything inactiveSessionIds.Verification
This PRoot/aarch64 environment cannot run the full Android build (the SDK ships x86-64
aapt2), so in addition to careful review I compiled the affected pure-JVM sources standalone and ran their unit tests directly:OpenCodeEventParserTest: 20/20 pass (incl. 2 new tests for the parsed parent link)RuntimeActivityRepositoryTest: 18/18 pass (incl. 4 new tests below)./gradlew spotlessCheck: passNew tests:
Pre-PR review
判定: APPROVE
ブロッカー
提案（非ブロッキング）
チェック済み項目
git diff origin/main...HEAD全体FakeTarget接続状態listSessions登録onSessionIdleの検証parentResolutionOf成功経路の検証git diff --check